Support in Set-GitHubContent to upload binary files #336

StanleyGoldman · 2021-09-07T19:06:19Z

Description

As mentioned in #335, this pull request adds functionality to upload binary content with Set-GitHubContent

Issues Fixed

References

Checklist

HowardWolosky

Welcome to the project @StanleyGoldman, and thanks for the idea and for the submission. You're off to a great start. I've provided some feedback for you to take a look at when you get a chance.

Thanks again.

GitHubContents.ps1

HowardWolosky · 2021-09-15T21:37:44Z

GitHubContents.ps1

@@ -322,7 +325,6 @@ filter Set-GitHubContent
        [string] $CommitMessage,

        [Parameter(
-            Mandatory,


This needs to remain mandatory in its own ParameterSet.

I think there was a misunderstanding with how I phrased this. It does need to be in its own parameter set, but not at the exclusion of the other required information. For instance, in order to what repo we're changing the content of, we still need to have the information either in the Elements parameter set or the Uri parameter set.

Previously, Content was being used with either of those parameter sets. Now you are introducing ContentPath as an alternative to Content. That means we're now exploded from 2 possible parameter sets to 4:

Elements with Content (maybe named ElementsContent)

Elements with ContentPath(maybe namedElementsContentPath`)

Uri with Content (maybe named UriContent)

Uri with ContentPath (maybe named UriContentPath)

You should try running Get-Help -Name Set-GitHubContent and look at the Syntax section. That will show you the various parameter sets that PowerShell has understood from the authored code. If you look at that with what you've currently authored, you should see more clearly how your new Content parameter set excludes the key information that we need, as well as prevents Content and ContentPath from being used when the repo information is provided.

Take a look at Get-GitHubEvent. That's an example where we had to explode the number of ParameterSets in order to support both the Elements and Uri repo scenarios while still accurately separating out the different possible inputs for the method itself.

HowardWolosky · 2021-09-15T21:39:22Z

GitHubContents.ps1

@@ -322,7 +325,6 @@ filter Set-GitHubContent
        [string] $CommitMessage,

        [Parameter(


We may also want to allow this to be ValueFromPipeline to enable the content to be piped directly into this method. That's certainly an optional change for this PR though.

Do you want to file an issue for this, or should I?

If you're not addressing this in this PR, then go ahead and open it up as a suggestion once this PR lands.

ghost · 2021-09-24T15:00:54Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 14 days of this comment.

Co-authored-by: Howard Wolosky <[email protected]>

HowardWolosky

Hey @StanleyGoldman,

Thanks so much for these updates, and my sincerest apologies for such a long time to get back to you with a follow-up review. I completely missed when you pushed up fixes earlier.

The main feedback here is still around the ParameterSets. Take a look at my more detailed comment in the reactivated comment thread to see if that gives you the additional information that you need to get it fixed.

HowardWolosky · 2022-04-29T18:41:56Z

GitHubContents.ps1

@@ -322,7 +325,6 @@ filter Set-GitHubContent
        [string] $CommitMessage,

        [Parameter(


If you're not addressing this in this PR, then go ahead and open it up as a suggestion once this PR lands.

HowardWolosky · 2022-04-29T18:54:23Z

GitHubContents.ps1

@@ -322,7 +325,6 @@ filter Set-GitHubContent
        [string] $CommitMessage,

        [Parameter(
-            Mandatory,


I think there was a misunderstanding with how I phrased this. It does need to be in its own parameter set, but not at the exclusion of the other required information. For instance, in order to what repo we're changing the content of, we still need to have the information either in the Elements parameter set or the Uri parameter set.

Previously, Content was being used with either of those parameter sets. Now you are introducing ContentPath as an alternative to Content. That means we're now exploded from 2 possible parameter sets to 4:

Elements with Content (maybe named ElementsContent)

Elements with ContentPath(maybe namedElementsContentPath`)

Uri with Content (maybe named UriContent)

Uri with ContentPath (maybe named UriContentPath)

You should try running Get-Help -Name Set-GitHubContent and look at the Syntax section. That will show you the various parameter sets that PowerShell has understood from the authored code. If you look at that with what you've currently authored, you should see more clearly how your new Content parameter set excludes the key information that we need, as well as prevents Content and ContentPath from being used when the repo information is provided.

HowardWolosky · 2022-04-29T19:06:44Z

GitHubContents.ps1

@@ -231,6 +231,9 @@ filter Set-GitHubContent
    .PARAMETER Content
        The new file content.

+    .PARAMETER ContentPath
+        The local path to file content.


Suggested change

The local path to file content.

The local path to a file whose content should replace what is currently stored remotely in the repo at the Path location.

HowardWolosky · 2022-04-29T19:11:47Z

GitHubContents.ps1

@@ -322,7 +325,6 @@ filter Set-GitHubContent
        [string] $CommitMessage,

        [Parameter(
-            Mandatory,


Take a look at Get-GitHubEvent. That's an example where we had to explode the number of ParameterSets in order to support both the Elements and Uri repo scenarios while still accurately separating out the different possible inputs for the method itself.

ghost · 2022-05-08T15:01:00Z

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 14 days of this comment.

ghost · 2022-05-29T15:01:02Z

This pull request has been automatically closed due to a lack of activity from the author. We understand. Life happens and other things likely came up. We would still love to see your contribution get merged in. Now that it has been closed, a different community member may wish to pick up where you left off. If so, they should speak up by commenting below. If you're still interested in completing this yourself, just respond back and let us know.

StanleyGoldman added 2 commits September 7, 2021 15:04

Adding parameter and functionality to upload a local path to GitHub

1b404ef

Formatting

38a20bd

HowardWolosky added api-contents Work to complete the API's defined here: https://developer.github.com/v3/repos/contents enhancement An issue or pull request introducing new functionality to the project. labels Sep 15, 2021

HowardWolosky requested changes Sep 15, 2021

View reviewed changes

ghost added the needs-author-feedback label Sep 15, 2021

ghost added the status-no-recent-activity label Sep 24, 2021

Apply suggestions from code review

1865d96

Co-authored-by: Howard Wolosky <[email protected]>

ghost removed needs-author-feedback status-no-recent-activity labels Sep 29, 2021

StanleyGoldman added 2 commits September 29, 2021 10:13

Changing parameter definition

909d8ea

Adding Test

13a260d

StanleyGoldman requested a review from HowardWolosky October 4, 2021 12:44

HowardWolosky requested changes Apr 29, 2022

View reviewed changes

ghost added the needs-author-feedback label Apr 29, 2022

ghost added the status-no-recent-activity label May 8, 2022

ghost closed this May 29, 2022

ghost added the auto-closed-unmerged An unmerged pull request that has been auto-closed due to lack of activity. label May 29, 2022

StanleyGoldman mentioned this pull request Aug 15, 2022

Support for Set-GitHubContent to Upload binary files #364

Closed

10 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support in Set-GitHubContent to upload binary files #336

Support in Set-GitHubContent to upload binary files #336

StanleyGoldman commented Sep 7, 2021 •

edited

Loading

HowardWolosky left a comment

HowardWolosky Sep 15, 2021

StanleyGoldman Sep 29, 2021

HowardWolosky Apr 29, 2022

HowardWolosky Apr 29, 2022

HowardWolosky Sep 15, 2021

StanleyGoldman Sep 29, 2021

HowardWolosky Apr 29, 2022

ghost commented Sep 24, 2021

HowardWolosky left a comment

HowardWolosky Apr 29, 2022

HowardWolosky Apr 29, 2022

HowardWolosky Apr 29, 2022

HowardWolosky Apr 29, 2022

ghost commented May 8, 2022

ghost commented May 29, 2022

		@@ -322,7 +325,6 @@ filter Set-GitHubContent
		[string] $CommitMessage,

		[Parameter(

	The local path to file content.
	The local path to a file whose content should replace what is currently stored remotely in the repo at the Path location.

Support in Set-GitHubContent to upload binary files #336

Support in Set-GitHubContent to upload binary files #336

Conversation

StanleyGoldman commented Sep 7, 2021 • edited Loading

Description

Issues Fixed

References

Checklist

HowardWolosky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Sep 24, 2021

HowardWolosky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented May 8, 2022

ghost commented May 29, 2022

StanleyGoldman commented Sep 7, 2021 •

edited

Loading